builtins: Allow numbers.range_step to use Decimal step size. - #172
builtins: Allow numbers.range_step to use Decimal step size.#172philipaconrad wants to merge 1 commit into
numbers.range_step to use Decimal step size.#172Conversation
This commit extends the `numbers.range_step` builtin to support `Decimal` step sizes. This is not as comprehensive a solution as bignum support, but it does increase the number of valid cases we can support from OPA, and will fix a broken test case in our Compliance test suite. Signed-off-by: Philip Conrad <philip_conrad@apple.com>
|
🚀! I noticed that the same |
| /// Whether this Decimal represents a whole number (no fractional part), regardless of | ||
| /// magnitude. Unlike ``safeInt64Value`` this does not require the value to fit in `Int64`, | ||
| /// so very large integers such as `99999999999999999999` still report `true`. | ||
| public var isWholeNumber: Bool { |
There was a problem hiding this comment.
Should we cover this in Tests/ASTTests/DecimalExtensionsTests.swift
There was a problem hiding this comment.
I think we could add some test for that. 🤔 I'll work that into my next round of changes for the PR.
|
|
||
| /// Whether this number is a whole number (no fractional part), regardless of magnitude. | ||
| /// Unlike ``int64Value`` this returns true even when the value is too large for `Int64`. | ||
| public var isInteger: Bool { |
There was a problem hiding this comment.
We have a series of tests that verify integerValue in Tests/ASTTests/RegoValueTests.swift. I would suggest adding unit tests there to cover isInteger and isPositive there
| /// result holds at most a handful of elements. We iterate in Decimal space (arbitrary | ||
| /// precision) to avoid Int64 overflow while remaining correct for steps that are only | ||
| /// slightly larger than Int64.max. | ||
| private static func rangeWithDecimalStep(intA: Int64, intB: Int64, step: Decimal) -> RegoValue { |
There was a problem hiding this comment.
Should we extend rangeStepTests in Tests/RegoTests/BuiltinTests/NumbersTests.swift with validations for of these big steps or do you think compliance tests are sufficient?
Ah! So the red No sync/async issues, just a confusingly-worded diff summary. I'll see about improving the readability of that message in a future PR. 😅 |
2 participants
What code changed, and why?
This PR extends the
numbers.range_stepbuiltin to supportDecimalstep sizes.OPA uses bignums from the Go stdlib to support large values for the range and step sizes, but we do not have nice bignum support in Swift at the moment.
While the solution here is not as comprehensive as bignums, it does increase the number of valid cases we can support from OPA, and will fix a broken test case in our Compliance test suite.
How to test
OPA_COMPLIANCE_TESTS='numbersrange' make test-complianceRelated Resources